[Feature] Add per-gpu mode switch in CLI#9
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request adds a useful feature for per-GPU fan mode control from the CLI. The implementation in src/main.c introduces new command-line argument parsing logic. While it works for the happy path, I've found some issues with error handling for invalid inputs, which could lead to confusing error messages for users. I've provided a detailed comment with a suggested refactoring to make the argument parsing more robust and maintainable. The documentation updates in README.md and src/display.c are clear.
|
Thanks for the contribution and the detailed explanation! I'll review the PR and test it on my setup tomorrow morning and get back to you with feedback. |
ricky-chaoju
left a comment
There was a problem hiding this comment.
Hi @delwiv, thanks for the contribution! The feature idea is great and useful. However there are some structural issues with the implementation that need to be addressed before merging. Please see inline comments below.
Summary of issues
-
Fragile control flow — The new per-GPU logic is placed inside the fallthrough
elseblock and relies onatoi()returning 0 for non-numeric strings like"auto"/"curve"to reach the new branches. This works by accident but is brittle and hard to follow. -
curvemode doesn't take effect immediately —autocallsfan_reset_to_auto()butcurveonly writes config without applying the fan curve. If the daemon isn't running, the command has no immediate effect. -
Redundant
atoi()call — In theargc == 4branch,gpu_index = atoi(argv[1])is called again even though it was already set at line 216. -
Help table formatting broken — The
manualrow is missing the|separator. -
Inconsistent error handling — Some error paths call
gpu_shutdown(); return 1;while others just fall through.
Suggested approach
Instead of appending new branches after the speed validation, handle mode keywords inside the existing argc == 3 block, before treating argv[2] as a speed:
} else if (argc == 3) {
gpu_index = atoi(argv[1]);
if (gpu_index >= 0 && gpu_index < (int)device_count) {
if (strcmp(argv[2], "auto") == 0) {
// ... per-GPU auto
} else if (strcmp(argv[2], "curve") == 0) {
// ... per-GPU curve
} else {
// original fixed speed logic
speed = atoi(argv[2]);
// ...
}
}
} else if (argc == 4) {
// nvfd <gpu_index> manual <speed>
// ...
}This makes the intent explicit and doesn't rely on atoi() side effects. Happy to discuss further!
|
@ricky-chaoju Thank you for the review, I'm going to check this later today |
…ual build instructions
|
@ricky-chaoju It got quite big from where it started, I took the freedom to add a new root folder : utils, to store the script and systemd unit, which can be optionally installed. The The original description were updated to reflect actual state of this PR. |
ricky-chaoju
left a comment
There was a problem hiding this comment.
Thanks for addressing all 5 issues from the previous review. The code is much cleaner now. A few remaining issues:
Issues
1. Silent failure when gpu_index is out of range (src/main.c)
In the argc == 3 block, if gpu_index is invalid (e.g. nvfd 99 auto), the code silently does nothing — no error message, no help output. The else branch for an invalid gpu_index is missing.
Please add an else branch that prints an error like "Invalid GPU index. Use 'nvfd list' to see available GPUs.".
2. Makefile — uninstall unconditionally depends on uninstall-utils
uninstall: uninstall-utilsThis means make uninstall always runs uninstall-utils, even if the user never installed utils. It's harmless now since it's just rm -f, but if uninstall-utils later adds systemctl disable or similar, it could cause issues. Consider removing the dependency and documenting make uninstall-utils as a separate step.
3. Minor: nvfd-fan-control.sh — no root check
The script writes to /var/run/nvfd-fan-control.lock which requires root, but there's no early root check. If run without root, the error message from exec 200>"$LOCKFILE" won't be very user-friendly.
The rest looks good. Nice work on the curve_apply_to_gpu() addition and the overall restructuring.
…nstall hard dependency on uninstall-utils and update (un)install scripts --with-utils. Update READMEs
|
@ricky-chaoju : I've processed your pertinent remarks, and have a few ones :
|
|
Thanks for addressing all the previous review items — the code is much cleaner now. A few remaining nits before we merge:
Regarding the utils uninstall tracking issue you raised — good catch, I agree that uninstalling main while keeping utils makes no sense and would leave a broken service. Let's handle that in a follow-up issue to keep this PR focused. |
|
LGTM! Thanks for the fix. |
Hello, I've added per-gpu mode switch with CLI interface, as I was missing this feature.
We cannot go under 30% fan speed, but my fans are pretty loud, so I wanted to switch to auto mode when temps is low (< 35-45°), and switch to curve when hitting this temp.
It was done with AI help (Qwen3.5 27B and Opencode), I insisted on being clear, not pollute existing code or docs, this is not AI slope.
Included an /utils directory to include script and systemd unit
Add Makefile targets for utils, and an arg to install script for optional utils
The Chinese README was updated by Qwen, which I believe, isn't too bad at that, but I can't guarantee it is correct.
Here is an AI summary :
Overview
Adds per-GPU fan mode switching via CLI (
nvfd <gpu_index> auto|curve|manual <speed>), enabling granular GPU fan control and temperature-aware automation.Key Changes
nvfd-fan-control.shwith hysteresis (threshold-up/down) to prevent mode thrashinginstall-utils/uninstall-utilstargetsAddresses Review Feedback
atoi()side effects)curve_apply_to_gpu()function)atoi()callsBackward Compatible
All existing commands work unchanged. Tested on dual-GPU system (RTX 2080 Ti + RTX 3090).
Installation